Skip to content
This repository has been archived by the owner on Jun 6, 2024. It is now read-only.

Done some Refactoring, like extract method, extract class, decomposing conditional etc. #489

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

BuzzLightyear2002
Copy link

@BuzzLightyear2002 BuzzLightyear2002 commented Apr 2, 2024

Thanks for submitting a pull request! Please check CONTRIBUTING.md for style guidelines.

Changes

Describe your changes here

New API Checklist

See CONTRIBUTING.md for more info.

  1. Documentation for every variable
  2. Class-level documentation
  3. POJO JSON parsing tests
  4. Service integration tests

Copy link

@jaDEVirek jaDEVirek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good with some nits.
Thanks.

// Constants for token counts per message and name
final int TOKENS_PER_MESSAGE_GPT_3_5_TURBO = 4;
final int TOKENS_PER_MESSAGE_GPT_4 = 3;
final int TOKENS_PER_NAME = 1;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final members for utils class, should be static.

Reason: It provides better performance, values are inlined at compile time instead of a runtime value lookup.

@@ -82,7 +82,7 @@ void createRetrieveRun() throws JsonProcessingException {


AssistantRequest assistantRequest = AssistantRequest.builder()
.model(TikTokensUtil.ModelEnum.GPT_4_1106_preview.getName())
.model(TikTokensUtil.ModelEnum.GPT_3_5_TURBO.getName())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any good reason, why the model type changed?
Thanks.

Map<JsonToken, JsonNodeHandler> handlers = new HashMap<>();
handlers.put(JsonToken.VALUE_NULL, new MissingNodeHandler());
// Add more handlers for different token types if needed
return handlers;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Returning collections such as Map - from functions, should preserve their immutability.
Thing about collections.unmodifiableMap
https://docs.oracle.com/javase/8/docs/api/java/util/Collections.html#unmodifiableMap-java.util.Map-

);
throw new OpenAiHttpException(error, e, e.code());
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: It can be optimized

try {
    ResponseBody errorBody = response.errorBody();
    if (errorBody != null) {
        OpenAiError error = mapper.readValue(errorBody.string(), OpenAiError.class);
        throw new OpenAiHttpException(error, new HttpException(response), response.code());
    }
} catch (IOException ex) {
    throw new HttpException(response);
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants